Would this allow for using the temperature to control a stepper driver fan? IMO this would be a nice use case instead unconditionally turning it on via controller_fan or just reporting the temperature.

Would this allow for using the temperature to control a stepper driver fan?

Yes! 🙂

That was exactly my point with this, one can now have temperature_fan using a tmc2240 XXX driver temperature monitored!

Thanks. As high-level feedback, I'm leery of introducing a "dummy temperature" to indicate "the temperature is not known". That is, I'm leery of using 0.0 when the driver temperature is not known. (In particular, when the dummy value itself could plausibly be valid.) I fear it could lead to confusion, weird looking graphs, and could propagate further downstream hacks to account for it.

-Kevin

Thanks Kevin, that's fair and makes complete sense.

I guess the alternative is returning None when the driver is disabled, but I believe Moonraker will then require changes to handle that (there's a round operation without any null check on it) - and I think same goes for Fluidd and Mainsail.

I will make the changes and test, and report back my findings.

Following up on the last comments, I've changed the code so that:

• PrinterTemperatureDriver.temp stores whatever the driver temperature read returns (so either a float or None)
• the callback is only called when we actually have a temperature read from the driver

Code performs as expected and looking at all the places in Klipper where temperature_driver can be used as a sensor, it seems ok that the callback is only called for non-None values.

@Arksine on a separate but related note, I'm already working on a PR for Moonraker, specifically it would allow for null/None temperature on the data_store.

I think the primary issue is that the TMC drivers don't consistently report temperature, thus they may not be a good candidate for a complete temperature_sensor implementation. I touched on this in the last PR which was why I recommended registering them as a monitor.

The challenge is determining the appropriate course of action when the driver is disabled. When the sensor is being relied upon by a peripheral this can introduce undesirable behavior, such as a continuously running fan.

That's a very good point, and I honestly didn't consider that possibility...

I admit I'm a bit out of ideas on how best to proceed on this PR, but @Arksine available_monitors proposal is starting to make a lot more sense as a way forward.

Thank you for your contribution to Klipper. Unfortunately, a reviewer has not assigned themselves to this GitHub Pull Request. All Pull Requests are reviewed before merging, and a reviewer will need to volunteer. Further information is available at: https://www.klipper3d.org/CONTRIBUTING.html

There are some steps that you can take now:

1. Perform a self-review of your Pull Request by following the steps at: https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review
   If you have completed a self-review, be sure to state the results of that self-review explicitly in the Pull Request comments. A reviewer is more likely to participate if the bulk of a review has already been completed.
2. Consider opening a topic on the Klipper Discourse server to discuss this work. The Discourse server is a good place to discuss development ideas and to engage users interested in testing. Reviewers are more likely to prioritize Pull Requests with an active community of users.
3. Consider helping out reviewers by reviewing other Klipper Pull Requests. Taking the time to perform a careful and detailed review of others work is appreciated. Regular contributors are more likely to prioritize the contributions of other regular contributors.

Unfortunately, if a reviewer does not assign themselves to this GitHub Pull Request then it will be automatically closed. If this happens, then it is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

@pedrolamas
The tmc2240 datasheet indicates that the driver also has a way to estimate the motor temperature (see bottom of pg. 65). Does your PR expose that function? I’m keen to use it, so if not or if you don’t plan to, I’ll contribute it after your change is accepted.

Thanks!

Hi @brotherdust, no this PR is just to expose the existing temperature read as a temperature sensor in Klipper, although it has some issues as reported above.

Though out of scope for this PR, the estimation does sound very interesting! I'll keep an eye out for a contribution PR from you for it!

@pedrolamas I think I will wait until the concerns raised above are addressed before submitting a PR (but will start working on it privately soon because I have some HOT steppers!)

Unfortunately a reviewer has not assigned themselves to this GitHub Pull Request and it is therefore being closed. It is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

I tried the configuration but I got the following error:

Unknown temperature sensor 'temperature_driver'

my config:

[stepper_x]
step_pin: STEP_1
dir_pin: !DIR_1
enable_pin: !EN_1
endstop_pin: !EndStop_1
microsteps: 64
rotation_distance: 40
# position_endstop: -12
position_endstop: -7
position_max: 300
# position_min: -12
position_min: -7
homing_speed: 50

[tmc2240 stepper_x]
cs_pin: UART_1
spi_software_sclk_pin: SCLK
spi_software_mosi_pin: MOSI
spi_software_miso_pin: MISO
interpolate = True
run_current: 0.8
# sense_resistor: 0.110
stealthchop_threshold: 500
# driver_IHOLDDELAY: 8
# driver_TPOWERDOWN: 20
# driver_TBL: 2
# driver_TOFF: 3
# driver_HEND: 0
# driver_HSTRT: 5
# driver_PWM_AUTOGRAD: True
# driver_PWM_AUTOSCALE: True
# driver_PWM_LIM: 12
# driver_PWM_REG: 8
# driver_PWM_FREQ: 1
# driver_PWM_GRAD: 14
# driver_PWM_OFS: 36
# driver_SGTHRS: 0

[temperature_sensor tmc2240_stepper_x]
sensor_type: temperature_driver
sensor_driver: tmc2240 stepper_x
# min_temp: 10
# max_temp: 100

@eldeeb91 this PR has not been merged, so the functionality here described is NOT available.

Having said that, #6292 was indeed merged, and together with changes that were also done in Moonraker, we have just released Fluidd 1.25 that shows the temperature of TMC2240 (screenshots here: fluidd-core/fluidd#1133)